-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Add logger package for node server side apps #6188
Conversation
WalkthroughThis pull request introduces several new files and configurations for the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Logger
Client->>Server: Sends HTTP request
Server->>Logger: requestLogger(req)
Logger-->>Server: Log request details
Server->>Client: Sends response
Logger-->>Logger: Log response status and duration
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
packages/logger/README.md (2)
1-4
: Enhance the introduction with installation instructionsThe README would benefit from a dedicated "Installation" section before the "Usage" section, detailing:
- npm/yarn installation commands
- Peer dependencies (if any)
- Node.js version requirements
12-20
: Fix package.json example syntaxThe package.json example contains a syntax error and could be more complete.
Consider updating to:
{ "dependencies": { "@plane/logger": "*" } }packages/logger/src/client-logger.ts (1)
31-36
: Enhance error handling for transportsCurrently, transport errors are logged to the console. Depending on the application's requirements, consider implementing additional error handling mechanisms such as alerting, retries, or fallback strategies to ensure robust logging.
packages/logger/src/server-logger.ts (1)
14-17
: Avoid using synchronous file system methods in a server environmentUsing
fs.existsSync
andfs.mkdirSync
blocks the event loop, which can impact performance. Consider using asynchronous methods to prevent blocking.Apply this diff to use asynchronous methods:
-if (!fs.existsSync(logDirectory)) { - fs.mkdirSync(logDirectory); - console.log('Logs folder created'); -} +fs.access(logDirectory, fs.constants.F_OK, (err) => { + if (err) { + fs.mkdir(logDirectory, { recursive: true }, (err) => { + if (err) { + console.error('Error creating logs folder:', err); + } else { + console.log('Logs folder created'); + } + }); + } +});packages/logger/src/logger.ts (1)
17-17
: Remove debuggingconsole.log
statementThe
console.log("inside client logger import")
statement appears to be for debugging purposes. It may be unnecessary in production code and could clutter the console output.Apply this diff to remove the debug statement:
- console.log("inside client logger import");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (11)
packages/logger/.eslintignore
(1 hunks)packages/logger/.eslintrc.js
(1 hunks)packages/logger/.prettierrc
(1 hunks)packages/logger/README.md
(1 hunks)packages/logger/package.json
(1 hunks)packages/logger/src/client-logger.ts
(1 hunks)packages/logger/src/index.ts
(1 hunks)packages/logger/src/logger.ts
(1 hunks)packages/logger/src/server-logger.ts
(1 hunks)packages/logger/tsconfig.json
(1 hunks)web/package.json
(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- packages/logger/src/index.ts
- packages/logger/.eslintignore
- packages/logger/.eslintrc.js
- packages/logger/.prettierrc
- packages/logger/tsconfig.json
- packages/logger/package.json
🧰 Additional context used
🪛 LanguageTool
packages/logger/README.md
[uncategorized] ~27-~27: Loose punctuation mark.
Context: ... from "@plane/logger"; ``` ### logger
: General Logger Use this for general app...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~48-~48: You might be missing the article “an” here.
Context: ...g the first import of logger by passing optional logLevel param in getLogger function.
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
🔇 Additional comments (1)
web/package.json (1)
37-37
: Consider using explicit versioning for production dependencies
While using "*"
for version matching is common for internal packages, it's worth verifying if this aligns with the versioning strategy used for other @plane/*
dependencies in this file.
✅ Verification successful
Versioning strategy is consistent across internal @plane packages
The use of "*"
version for @plane/logger
aligns with the versioning strategy used for all other internal @plane/*
dependencies in the package.json file. This appears to be an intentional pattern for managing internal package dependencies within the monorepo.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check versioning pattern of other @plane/* dependencies
rg '"@plane/[^"]+": "[^"]+"' web/package.json
Length of output: 269
packages/logger/src/client-logger.ts
Outdated
private static getInstance(logLevel?: string) { | ||
if (!ClientLogger.instance) { | ||
ClientLogger.instance = new ClientLogger(logLevel); | ||
} | ||
return ClientLogger.instance; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Singleton pattern may ignore subsequent logLevel
changes
The getInstance
method initializes the logger only once. Subsequent calls to getLogger
with a different logLevel
will not update the log level of the existing logger instance, which may lead to unexpected behavior if different log levels are specified later.
Consider modifying the implementation to allow updating the log level dynamically or document that the log level is set only during the first initialization.
Apply this diff to update the logger's level if a new logLevel
is provided:
public static getLogger(logLevel?: string) {
- const instance = ClientLogger.getInstance(logLevel);
+ const instance = ClientLogger.getInstance();
+ if (logLevel && logLevel !== instance.logLevel) {
+ instance.logger.level = logLevel;
+ instance.logLevel = logLevel;
+ }
return instance.logger;
}
Committable suggestion skipped: line range outside the PR's diff.
packages/logger/src/server-logger.ts
Outdated
private static getInstance(logLevel?: string, logFilePrefix?: string) { | ||
if (!Logger.instance) { | ||
Logger.instance = new Logger(logLevel, logFilePrefix); | ||
} | ||
return Logger.instance; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Singleton pattern may ignore subsequent logLevel
and logFilePrefix
changes
The getInstance
method initializes the logger once. Subsequent calls to getLogger
with different logLevel
or logFilePrefix
will not update the existing logger instance, which may cause unexpected behavior if these parameters are changed later.
Consider modifying the implementation to handle dynamic updates to logLevel
and logFilePrefix
or document that these parameters are set only during the first initialization.
Apply this diff to update the logger's configuration if new parameters are provided:
public static getLogger(logLevel?: string, logFilePrefix?: string) {
- const instance = Logger.getInstance(logLevel, logFilePrefix);
+ const instance = Logger.getInstance();
+ if (logLevel && logLevel !== instance.logLevel) {
+ instance.logger.level = logLevel;
+ instance.logLevel = logLevel;
+ }
+ if (logFilePrefix && logFilePrefix !== instance.logFilePrefix) {
+ // Logic to update the logFilePrefix, which may require reconfiguring transports
+ instance.logFilePrefix = logFilePrefix;
+ // Reconfigure or recreate transports as necessary
+ }
return instance.logger;
}
Note: Updating the logFilePrefix
may require recreating the transport to apply the new file naming convention.
Committable suggestion skipped: line range outside the PR's diff.
packages/logger/src/logger.ts
Outdated
if (typeof window !== "undefined") { | ||
// Client-side logic | ||
console.log("inside client logger import") | ||
Logger = require('./client-logger').default; | ||
} else { | ||
// Server-side logic | ||
Logger = require('./server-logger').default; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixing ES modules and CommonJS require
statements
The code uses require
statements in a module that uses import
statements. This can lead to compatibility issues, especially when using TypeScript with ES module syntax. Consider using dynamic import()
for conditional imports.
Apply this diff to replace require
with dynamic imports:
let Logger: ILogger;
-if (typeof window !== "undefined") {
- // Client-side logic
- console.log("inside client logger import")
- Logger = require('./client-logger').default;
-} else {
- // Server-side logic
- Logger = require('./server-logger').default;
-}
+if (typeof window !== "undefined") {
+ // Client-side logic
+ console.log("inside client logger import");
+ import('./client-logger').then((module) => {
+ Logger = module.default;
+ });
+} else {
+ // Server-side logic
+ import('./server-logger').then((module) => {
+ Logger = module.default;
+ });
+}
Alternatively, ensure that the module system is consistent throughout the project.
Committable suggestion skipped: line range outside the PR's diff.
packages/logger/src/server-logger.ts
Outdated
private logLevel: string; | ||
private logFilePrefix: string; | ||
|
||
private constructor(logLevel: string = "info", logFilePrefix: string = "plane-log") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
convert those log level and log file prefix as entities inside an object.
Basically constructor will take a object, you destruct it and use it.
By doing this like and server logger can be initialised with same type of object.
packages/logger/package.json
Outdated
"dist/**" | ||
], | ||
"scripts": { | ||
"build": "tsup ./src/index.ts --format esm,cjs --dts --external react --minify", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
donot create a build for this package. You can directly import them and use it. Let the application that is using it transpile these packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
packages/logger/README.md (2)
27-27
: Fix Markdown formatting for the headingThere is no blank line between the code block and the heading at line 27, which may cause rendering issues. Please add a blank line between the code block and the heading.
Apply this diff:
} +``` ### `logger`: General Logger Use this for general application logs.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~27-~27: Loose punctuation mark.
Context: ... from "@plane/logger"; ``` ###logger
: General Logger Use this for general app...(UNLIKELY_OPENING_PUNCTUATION)
58-58
: Add missing article "an"Line 58 is missing the article "an" before "optional". It should read "by passing an optional
logLevel
param".Apply this diff:
- You can specify a log level during the first import of logger by passing optional logLevel param in getLogger function. + You can specify a log level during the first import of the logger by passing an optional `logLevel` param in the `getLogger` function.🧰 Tools
🪛 LanguageTool
[uncategorized] ~58-~58: You might be missing the article “an” here.
Context: ...g the first import of logger by passing optional logLevel param in getLogger function.(AI_EN_LECTOR_MISSING_DETERMINER_AN)
packages/logger/src/server-logger.ts (1)
15-18
: Avoid synchronous file system operationsUsing synchronous file system methods like
fs.existsSync
andfs.mkdirSync
can block the event loop and impact performance. Consider using asynchronous methods instead.Apply this diff:
-if (!fs.existsSync(logDirectory)) { - fs.mkdirSync(logDirectory); - console.log('Logs folder created'); -} +fs.promises.mkdir(logDirectory, { recursive: true }) + .then(() => console.log('Logs folder created')) + .catch((err) => { + if (err.code !== 'EEXIST') { + console.error('Error creating logs folder:', err); + } + });Ensure that any code depending on the existence of the log directory accounts for the asynchronous nature of this operation.
packages/logger/tsconfig.json (1)
4-6
: Consider using a more conservative target for better compatibility.While using
ESNext
for bothmodule
andtarget
provides access to the latest features, it might limit compatibility with older Node.js versions. Consider using explicit versions (e.g.,ES2020
) based on your minimum supported Node.js version.- "module": "ESNext", - "target": "ESNext", + "module": "ES2020", + "target": "ES2020",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/logger/README.md
(1 hunks)packages/logger/index.d.ts
(1 hunks)packages/logger/package.json
(1 hunks)packages/logger/src/client-logger.ts
(1 hunks)packages/logger/src/index.ts
(1 hunks)packages/logger/src/logger.ts
(1 hunks)packages/logger/src/server-logger.ts
(1 hunks)packages/logger/tsconfig.json
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/logger/src/logger.ts
- packages/logger/src/index.ts
- packages/logger/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/logger/index.d.ts
[error] 6-6: ';' expected'
An explicit or implicit semicolon is expected here...
(parse)
[error] 7-7: ';' expected'
An explicit or implicit semicolon is expected here...
(parse)
packages/logger/src/client-logger.ts
[error] 28-28: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🪛 LanguageTool
packages/logger/README.md
[uncategorized] ~27-~27: Loose punctuation mark.
Context: ... from "@plane/logger"; ``` ### logger
: General Logger Use this for general app...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~58-~58: You might be missing the article “an” here.
Context: ...g the first import of logger by passing optional logLevel param in getLogger function.
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
🔇 Additional comments (6)
packages/logger/src/client-logger.ts (1)
20-25
:
Singleton pattern ignores subsequent logLevel
changes
The getInstance
method initializes the logger only once. Subsequent calls to getLogger
with different logLevel
values will not update the log level of the existing logger instance, which may lead to unexpected behavior if different log levels are specified later.
Consider modifying the implementation to allow updating the log level dynamically.
Apply this diff to update the logger's level if a new logLevel
is provided:
public static getLogger(loggerOptions?: ILoggerOptions): IClientLogMethods {
- const instance = ClientLogger.getInstance(loggerOptions);
+ const instance = ClientLogger.getInstance();
+ if (loggerOptions?.logLevel && loggerOptions.logLevel !== instance.logLevel) {
+ instance.logLevel = loggerOptions.logLevel;
+ }
return instance.logMethods;
}
Also applies to: 27-30
packages/logger/README.md (1)
56-58
: Consider adding production configuration guidelines
Including guidance on production configurations can help users set up the logger effectively in different environments. This might cover best practices, log rotation settings, environment-specific configurations, and performance considerations.
Would you like assistance in providing detailed examples of production configuration guidelines?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~58-~58: You might be missing the article “an” here.
Context: ...g the first import of logger by passing optional logLevel param in getLogger function.
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
packages/logger/src/server-logger.ts (1)
75-80
:
Singleton pattern ignores subsequent loggerOptions
changes
The getInstance
method initializes the logger only once. Subsequent calls to getLogger
with different loggerOptions
will not update the existing logger instance. This may lead to unexpected behavior if different logLevel
or logFilePrefix
values are specified later.
Consider modifying the implementation to update the logger's configuration if new loggerOptions
are provided.
Apply this diff to update the logger's configuration dynamically:
private static getInstance() {
if (!Logger.instance) {
Logger.instance = new Logger();
}
return Logger.instance;
}
public static getLogger(loggerOptions?: ILoggerOptions) {
const instance = Logger.getInstance();
+ if (loggerOptions?.logLevel && loggerOptions.logLevel !== instance.logLevel) {
+ instance.logger.level = loggerOptions.logLevel;
+ instance.logLevel = loggerOptions.logLevel;
+ }
+ if (loggerOptions?.logFilePrefix && loggerOptions.logFilePrefix !== instance.logFilePrefix) {
+ // Logic to update the logFilePrefix
+ instance.logFilePrefix = loggerOptions.logFilePrefix;
+ // Reconfigure transports as necessary
+ }
return instance.logger;
}
Note: Updating the logFilePrefix
may require recreating the transport to apply the new file naming convention.
packages/logger/tsconfig.json (3)
17-18
: LGTM! Source file patterns are properly configured.
The include/exclude patterns are well-defined, including all source files while properly excluding node_modules.
2-2
: Verify the base TypeScript configuration.
Let's ensure the base configuration file exists and is properly set up.
✅ Verification successful
Base TypeScript configuration is properly set up and accessible
The base configuration exists at packages/typescript-config/base.json
and provides a solid foundation with:
- Modern module resolution (NodeNext)
- Strict type checking
- Declaration file generation
- Essential compiler safeguards
The configuration is properly published as part of the @plane/typescript-config
package and the logger's tsconfig.json correctly extends it.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and content of the base TypeScript configuration
# Check if the base config exists
fd -t f "base.json" -p "@plane/typescript-config"
# If found, examine its content
fd -t f "base.json" -p "@plane/typescript-config" -x cat {}
Length of output: 331
Script:
#!/bin/bash
# Let's try different approaches to locate the TypeScript configuration
# 1. Find all tsconfig files to understand the structure
fd tsconfig
# 2. Look for any package.json that might reference typescript-config
fd "package.json" -x grep -l "typescript-config"
# 3. Search for base.json in the entire repository
fd "base.json"
Length of output: 494
Script:
#!/bin/bash
# Let's examine the base.json content and verify it's properly configured
cat packages/typescript-config/base.json
# Also check the package.json of typescript-config to understand its setup
cat packages/typescript-config/package.json
Length of output: 801
11-13
: Verify path alias configuration in build setup.
The path alias @/*
needs corresponding configuration in your build tools (e.g., webpack, rollup, etc.) to work correctly.
✅ Verification successful
Let me gather more information about the build setup to verify the path alias configuration.
Let me check the tsup configuration since it's being used as the build tool.
Path alias configuration is properly handled by tsup
The project uses tsup as the build tool, which automatically handles TypeScript path aliases from tsconfig.json during the build process. No additional build configuration is needed as tsup inherently supports TypeScript's path mapping feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for build configuration files that might need path alias setup
# Search for common build config files
fd -t f "webpack.config|rollup.config|vite.config|package.json" -p "packages/logger"
# Check if path aliases are configured in package.json
fd -t f "package.json" -p "packages/logger" -x jq '.exports' {}
Length of output: 390
Script:
#!/bin/bash
# Check for build configuration in package.json and any build scripts
fd -t f "package.json" -p "packages/logger" -x jq '.scripts' {}
# Look for tsconfig references in the project
fd -t f "tsconfig" -p "packages/logger"
# Check if there's any build tool configuration
fd -t f "rollup|webpack|vite|babel|esbuild" -p "packages/logger"
Length of output: 373
Script:
#!/bin/bash
# Check if there's a tsup config file
fd -t f "tsup.config" -p "packages/logger"
# Look at the actual source files to see if path aliases are being used
rg -g "*.{ts,tsx}" "from '@/" packages/logger/src
Length of output: 95
packages/logger/src/client-logger.ts
Outdated
} | ||
|
||
public static getLogger(loggerOptions?: ILoggerOptions): IClientLogMethods { | ||
const instance = this.getInstance(loggerOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Use class name instead of 'this' in static method
In static methods, using this
can be confusing because it refers to the class itself. Replace this
with the class name ClientLogger
for clarity.
Apply this diff:
public static getLogger(loggerOptions?: ILoggerOptions): IClientLogMethods {
- const instance = this.getInstance(loggerOptions);
+ const instance = ClientLogger.getInstance(loggerOptions);
return instance.logMethods;
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const instance = this.getInstance(loggerOptions); | |
const instance = ClientLogger.getInstance(loggerOptions); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
packages/logger/src/config.ts (4)
5-12
: Add JSDoc comments for log levelsConsider adding JSDoc comments to document the purpose and usage of each log level. This would help other developers understand when to use each level appropriately.
// Define log levels +/** + * Log levels in order of increasing verbosity: + * - error: For errors that need immediate attention + * - warn: For potentially harmful situations + * - info: For general information about system operation + * - http: For HTTP request logging + * - debug: For detailed debugging information + */ const levels = {
14-24
: Add TypeScript type definitions for colorsConsider adding a type definition for the colors object to improve type safety.
+type LogColors = { + [K in keyof typeof levels]: string; +}; + // Define colors for each level -const colors = { +const colors: LogColors = { error: "red", warn: "yellow", info: "green", http: "magenta", debug: "white", };
26-33
: Consider enhancing the log formatThe current format is clean but could be more informative for debugging purposes. Also, colorizing all content might interfere with log parsing tools.
const format = winston.format.combine( winston.format.timestamp({ format: "YYYY-MM-DD HH:mm:ss:ms" }), - winston.format.colorize({ all: true }), + winston.format.colorize({ level: true }), winston.format.printf( - (info: winston.Logform.TransformableInfo) => `[${info?.timestamp}] ${info.level}: ${info.message}` + (info: winston.Logform.TransformableInfo) => `[${info?.timestamp}] ${info.level}: ${info.message}${info.stack ? '\n' + info.stack : ''}${ + Object.keys(info).some(key => !['timestamp', 'level', 'message', 'stack'].includes(key)) + ? '\n' + JSON.stringify(Object.assign({}, info, { timestamp: undefined, level: undefined, message: undefined, stack: undefined })) + : '' + }` ) );
1-66
: Add tests and usage documentationConsider adding:
- Unit tests for logger configuration
- Integration tests for file rotation
- Usage examples in the README
- Documentation for available environment variables
This is a critical infrastructure component that needs thorough testing and documentation.
Would you like me to help create:
- A test suite template?
- Usage examples for the README?
- Environment variables documentation?
packages/logger/src/middleware.ts (1)
1-2
: Consider adding type safety for the logger interfaceWhile the Express types are properly imported, we should ensure type safety for the logger interface to prevent potential runtime errors if the logger implementation changes.
import { Request, Response, NextFunction } from "express"; +import type { Logger } from 'winston'; import { logger } from "./config";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/logger/package.json
(1 hunks)packages/logger/src/config.ts
(1 hunks)packages/logger/src/index.ts
(1 hunks)packages/logger/src/middleware.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/logger/package.json
- packages/logger/src/index.ts
🔇 Additional comments (2)
packages/logger/src/config.ts (1)
1-3
: Verify package dependencies and add type definitions
Please ensure these dependencies are properly listed in package.json. Also, consider adding type definitions for winston-daily-rotate-file.
packages/logger/src/middleware.ts (1)
4-7
: 🛠️ Refactor suggestion
Add error handling wrapper
The middleware should be wrapped in a try-catch block to prevent unhandled exceptions from crashing the application.
export const requestLogger = (req: Request, res: Response, next: NextFunction) => {
+ try {
// Log when the request starts
const startTime = Date.now();
+ } catch (error) {
+ logger.error('Error in request logger middleware:', error);
+ next(error);
+ }
Likely invalid or redundant comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/logger/README.md (3)
5-5
: Remove trailing period from headingRemove the trailing period from the "Features" heading to follow markdown conventions.
-## Features. +## Features🧰 Tools
🪛 Markdownlint (0.37.0)
5-5: Punctuation: '.'
Trailing punctuation in heading(MD026, no-trailing-punctuation)
3-3
: Enhance the introduction with structured logging detailsConsider expanding the introduction to highlight the structured logging capabilities, including:
- JSON log format
- Correlation ID support
- Log metadata enrichment
55-55
: Fix hyphenation in rotation periodAdd a hyphen to the compound adjective.
-Log files have a 7 day rotation period defined. +Log files have a 7-day rotation period defined.🧰 Tools
🪛 LanguageTool
[uncategorized] ~55-~55: When a number forms part of an adjectival compound, use a hyphen.
Context: ...mbined-%DATE%.log`. - Log files have a 7 day rotation period defined. ## Configurat...(MISSING_HYPHEN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/logger/README.md
(1 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/logger/README.md
[uncategorized] ~36-~36: Loose punctuation mark.
Context: ... is an error"); ``` ### requestLogger
: Request Logger Middleware Use this as a...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~55-~55: When a number forms part of an adjectival compound, use a hyphen.
Context: ...mbined-%DATE%.log`. - Log files have a 7 day rotation period defined. ## Configurat...
(MISSING_HYPHEN)
🪛 Markdownlint (0.37.0)
packages/logger/README.md
5-5: Punctuation: '.'
Trailing punctuation in heading
(MD026, no-trailing-punctuation)
🔇 Additional comments (3)
packages/logger/README.md (3)
30-34
: Add error handling examples and type information
The current examples are too basic. They should demonstrate:
- Error handling with proper typing
- Structured logging with metadata
- Async logging scenarios
57-59
: Add production configuration guidance
The configuration section should include production best practices, log rotation settings, and environment-specific configurations.
7-8
: Verify logger implementation matches documentation
Let's verify that the actual implementation supports all documented features.
✅ Verification successful
Let me gather more information about the winston logger configuration since the first pattern didn't show the complete implementation.
Let me check the complete content of the logger configuration file to verify all the features.
Documentation accurately reflects the logger implementation
The implementation fully supports the documented features:
- Pre-configured winston logger is properly implemented with comprehensive configuration including:
- Multiple log levels (error, warn, info, http, debug)
- Console and rotating file transports
- Configurable log level via environment variable
- Request logger middleware is implemented with detailed request/response logging capabilities
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify logger implementation matches documentation
# Check if winston logger is properly configured
ast-grep --pattern 'import { createLogger } from "winston"'
# Verify log level configuration from env
ast-grep --pattern 'process.env.LOG_LEVEL'
# Check request logger middleware implementation
ast-grep --pattern $'export const requestLogger = (req: Request, res: Response, next: NextFunction) => {
$$$
}'
Length of output: 1623
Script:
#!/bin/bash
# Check the complete winston logger configuration
ast-grep --pattern $'export const logger = createLogger({
$$$
})'
# Also check if there's a general usage export
rg -l "export.*logger" packages/logger/src/
Length of output: 144
Script:
#!/bin/bash
# Get the complete logger configuration
cat packages/logger/src/config.ts
Length of output: 1521
Description
Created a node package for logger as a wrapper around winston with configurable log level. This can be used for any node server side application.
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
Imported the package in a server side application and tested the logs
Summary by CodeRabbit
Release Notes
New Features
@plane/logger
) with a singleton-based logging utility.Documentation
README.md
for the Logger Package detailing usage and features.Configuration
Bug Fixes